feat(gen_sim): add Scene Engine and Gradio workspace - #457
Conversation
…ain into muzi/feat_scene_engine
…tion algo + Reformatted the code
…he 2d table aabb index place on the gray asset mask
# Conflicts: # .github/workflows/main.yml # embodichain/gen_sim/scene_engine/pipeline/utils/scene_exporter.py # pyproject.toml # tests/gen_sim/scene_engine/test_scene_core_and_export.py
Greptile SummaryThis PR adds shared GenSim environment loading and a Gradio workspace for Scene Engine, SimReady, Articraft generation, and Viser previews.
Confidence Score: 4/5The PR is not safe to merge until the missing-environment startup crash, cross-session process cancellation, unrelated-listener termination, and Codex credential exposure are fixed. The default workspace startup fails when no local Files Needing Attention: embodichain/gen_sim/env.py, embodichain/gen_sim/gradio_ui/app_articraft.py, embodichain/gen_sim/gradio_ui/app_asset_engine.py, embodichain/gen_sim/gradio_ui/gradio_app.py
|
| Filename | Overview |
|---|---|
| embodichain/gen_sim/env.py | Adds shared dotenv parsing, but the missing-file path dereferences an implicit None and prevents normal startup. |
| embodichain/gen_sim/gradio_ui/app_articraft.py | Adds the Articraft generation and preview workflow, including unsafe credential inheritance and unverified termination of occupied-port listeners. |
| embodichain/gen_sim/gradio_ui/app_asset_engine.py | Adds SimReady UI execution, but global process ownership lets a queue-bypassing reset cancel another session's run. |
| embodichain/gen_sim/gradio_ui/gradio_app.py | Launches the queued workspace on all interfaces without built-in authentication, making shared-state and agent-boundary defects reachable by multiple clients. |
| embodichain/main.py | Registers the Scene Engine generation and preview commands through lazy CLI dispatch. |
Sequence Diagram
sequenceDiagram
participant User as Gradio user
participant UI as Gradio workspace
participant Env as Shared environment
participant Job as Pipeline/Codex process
participant Preview as Viser preview
User->>UI: Submit image, asset, or prompt
UI->>Env: Load GenSim configuration
UI->>Job: Start managed generation process
Job-->>UI: Stream logs and generated outputs
UI->>Preview: Start scene or asset preview
Preview-->>User: Embedded interactive visualization
User->>UI: Reset workflow
UI->>Job: Terminate globally tracked process
Prompt To Fix All With AI
### Issue 1
embodichain/gen_sim/env.py:69-71
**Missing dotenv crashes startup**
When neither `EMBODICHAIN_ENV_FILE` nor the optional local `.env` exists, `find_gen_sim_env_file()` returns `None` and the loader calls `.is_file()` on it, causing the Gradio workspace to fail during import with an `AttributeError`.
```suggestion
env_path = find_gen_sim_env_file()
if env_path is None or not env_path.is_file():
return None
```
### Issue 2
embodichain/gen_sim/gradio_ui/app_articraft.py:566
**Preview kills unowned listeners**
If another same-user service occupies `ARTICRAFT_VISER_PORT`, preview startup sends SIGTERM and potentially SIGKILL to every discovered listener without verifying application ownership, taking the unrelated service offline.
**How this was verified:** The occupied-port path leads from unfiltered listener PID discovery directly to `os.kill`.
### Issue 3
embodichain/gen_sim/gradio_ui/app_articraft.py:853-861
**Codex inherits server credentials**
When a workspace user instructs Codex to print an environment credential, the user-directed process receives the complete dotenv-populated `os.environ` and its combined output is streamed to the browser, disclosing the credential in the generation log.
**How this was verified:** The user prompt reaches a command-capable Codex process that inherits the full server environment and returns captured stdout to the UI.
### Issue 4
embodichain/gen_sim/gradio_ui/app_asset_engine.py:84-92
**Reset cancels another session**
If one session clicks Reset while another session has a SimReady or Articraft job running, the queue-bypassing callback clears module-global ownership state and terminates the shared process, causing the first session's generator to exit without its requested result.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "add log" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| env_path = find_gen_sim_env_file() | ||
| if not env_path.is_file(): | ||
| return None |
There was a problem hiding this comment.
When neither EMBODICHAIN_ENV_FILE nor the optional local .env exists, find_gen_sim_env_file() returns None and the loader calls .is_file() on it, causing the Gradio workspace to fail during import with an AttributeError.
| env_path = find_gen_sim_env_file() | |
| if not env_path.is_file(): | |
| return None | |
| env_path = find_gen_sim_env_file() | |
| if env_path is None or not env_path.is_file(): | |
| return None |
Prompt To Fix With AI
This is a comment left during a code review.
Path: embodichain/gen_sim/env.py
Line: 69-71
Comment:
**Missing dotenv crashes startup**
When neither `EMBODICHAIN_ENV_FILE` nor the optional local `.env` exists, `find_gen_sim_env_file()` returns `None` and the loader calls `.is_file()` on it, causing the Gradio workspace to fail during import with an `AttributeError`.
```suggestion
env_path = find_gen_sim_env_file()
if env_path is None or not env_path.is_file():
return None
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| f"Port {self._port} is unavailable without a visible listener." | ||
| ) | ||
|
|
||
| self._signal_listeners(listener_pids, signal.SIGTERM, "stop") |
There was a problem hiding this comment.
If another same-user service occupies ARTICRAFT_VISER_PORT, preview startup sends SIGTERM and potentially SIGKILL to every discovered listener without verifying application ownership, taking the unrelated service offline.
How this was verified: The occupied-port path leads from unfiltered listener PID discovery directly to os.kill.
Prompt To Fix With AI
This is a comment left during a code review.
Path: embodichain/gen_sim/gradio_ui/app_articraft.py
Line: 566
Comment:
**Preview kills unowned listeners**
If another same-user service occupies `ARTICRAFT_VISER_PORT`, preview startup sends SIGTERM and potentially SIGKILL to every discovered listener without verifying application ownership, taking the unrelated service offline.
**How this was verified:** The occupied-port path leads from unfiltered listener PID discovery directly to `os.kill`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| subprocess.Popen( | ||
| codex_command, | ||
| cwd=ARTICRAFT_ROOT, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.STDOUT, | ||
| text=True, | ||
| bufsize=1, | ||
| start_new_session=True, | ||
| env=os.environ.copy(), |
There was a problem hiding this comment.
When a workspace user instructs Codex to print an environment credential, the user-directed process receives the complete dotenv-populated os.environ and its combined output is streamed to the browser, disclosing the credential in the generation log.
How this was verified: The user prompt reaches a command-capable Codex process that inherits the full server environment and returns captured stdout to the UI.
Prompt To Fix With AI
This is a comment left during a code review.
Path: embodichain/gen_sim/gradio_ui/app_articraft.py
Line: 853-861
Comment:
**Codex inherits server credentials**
When a workspace user instructs Codex to print an environment credential, the user-directed process receives the complete dotenv-populated `os.environ` and its combined output is streamed to the browser, disclosing the credential in the generation log.
**How this was verified:** The user prompt reaches a command-capable Codex process that inherits the full server environment and returns captured stdout to the UI.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| def reset_simready_asset(): | ||
| """Clear SimReady widgets and terminate the process group for its active run.""" | ||
| global _simready_process, _simready_run_token | ||
| with _simready_run_lock: | ||
| process = _simready_process | ||
| _simready_process = None | ||
| _simready_run_token = None | ||
| if process is not None: | ||
| terminate_process_group(process) |
There was a problem hiding this comment.
If one session clicks Reset while another session has a SimReady or Articraft job running, the queue-bypassing callback clears module-global ownership state and terminates the shared process, causing the first session's generator to exit without its requested result.
Prompt To Fix With AI
This is a comment left during a code review.
Path: embodichain/gen_sim/gradio_ui/app_asset_engine.py
Line: 84-92
Comment:
**Reset cancels another session**
If one session clicks Reset while another session has a SimReady or Articraft job running, the queue-bypassing callback clears module-global ownership state and terminates the shared process, causing the first session's generator to exit without its requested result.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| target="embodichain.workspace_cache_cli:main", | ||
| help="Inspect and clean workspace analyzer caches.", | ||
| ), | ||
| Command( |
yuecideng
left a comment
yuecideng
left a comment
There was a problem hiding this comment.
Review focused on the new Gradio workspace. The inline comments cover security, data-loss, runtime, packaging, and CI blockers. Local validation also found that pytest tests/test_main.py fails because analyze-workspace is no longer registered, while pytest tests/gen_sim fails during collection because of the duplicate test_config.py module name. The existing inline thread on embodichain/__main__.py already calls out restoring that command, so I did not duplicate it.
| server_name=SERVER_NAME, | ||
| server_port=SERVER_PORT, | ||
| allowed_paths=[ | ||
| str(EMBODICHAIN_ROOT), |
There was a problem hiding this comment.
Blocking security issue: this exposes the entire repository through Gradio while the server defaults to 0.0.0.0 and no authentication is configured. Gradio treats every file below an allowed directory as publicly servable, so this includes embodichain/gen_sim/.env and its API credentials. Please whitelist only the generated artifact/assets directories, explicitly block secret paths, and either bind to localhost by default or require authentication.
| key=lambda item: len(item.parts), | ||
| reverse=True, | ||
| ): | ||
| if path.is_file() and path.suffix.lower() not in VIDEO_SUFFIXES: |
There was a problem hiding this comment.
OUTPUTS_DIR is the repository-wide outputs/ directory, so this loop deletes every non-video file below it whenever Reset or Auto cleanup runs. That includes unrelated RL checkpoints, debug reports, trajectories, and datasets. Please track and remove only artifacts created by the current Gradio run instead of recursively cleaning this shared directory.
|
|
||
| def configured_lerobot_roots() -> list[Path]: | ||
| roots: list[Path] = [] | ||
| env_root = os.environ.get("EMBODICHAIN_DATASET_ROOT") |
There was a problem hiding this comment.
This function cannot run: os is not imported, and CURRENT_PATHS referenced below is not defined or imported by this module either. monitor_simulation() calls this path after DexSim exits but before clearing runtime.sim_process, so every completion raises NameError and leaves the UI/Auto loop stuck in a running state. Please import or pass these dependencies explicitly and make the runtime cleanup execute in a finally path.
| f"Port {self._port} is unavailable without a visible listener." | ||
| ) | ||
|
|
||
| self._signal_listeners(listener_pids, signal.SIGTERM, "stop") |
There was a problem hiding this comment.
This treats every process listening on the configured port as a stale Articraft preview and sends it SIGTERM, later escalating to SIGKILL, without verifying ownership. A normal port collision can therefore terminate an unrelated user service. Please terminate only self._process (or another registered child owned by this app); otherwise report that the port is already in use.
| ValueError: If the file contains an invalid ``KEY=VALUE`` entry. | ||
| """ | ||
| target_env = os.environ if env is None else env | ||
| env_path = find_gen_sim_env_file() |
There was a problem hiding this comment.
When neither EMBODICHAIN_ENV_FILE nor embodichain/gen_sim/.env exists, find_gen_sim_env_file() falls through and returns None, so this immediately raises AttributeError: 'NoneType' object has no attribute 'is_file'. A clean checkout only contains .env.example. Please return Path | None and guard None here, or always return the documented fallback path.
| def build_pipeline_env() -> dict[str, str]: | ||
| env = os.environ.copy() | ||
| configure_direct_network_env(env) | ||
| configure_simready_llm_env(env) |
There was a problem hiding this comment.
build_pipeline_env() is used by Scene Engine, DexSim, Viser, and Articraft preview processes as well as SimReady, but this unconditionally maps SIMREADY_OPENAI_* over OPENAI_*. If separate endpoints are configured as supported by .env.example, Scene Engine will receive the SimReady model, URL, and key. Please apply this mapping only to the SimReady command.
| return "stopped" | ||
|
|
||
| with runtime_lock: | ||
| simulation_completed = ( |
There was a problem hiding this comment.
sim_returncode is recorded by the monitor but never checked here. A non-zero DexSim exit still satisfies sim_started && sim_finished && sim_process is None, so Auto records the round as completed and continues to later phases. Please require runtime.sim_returncode == 0 and also propagate a non-zero exit to the failed phase/last_error.
|
|
||
| import pytest | ||
|
|
||
| from embodichain.gen_sim.scene_engine.cli import start |
There was a problem hiding this comment.
This new test module has the same basename as tests/gen_sim/simready_pipeline/test_config.py, and neither parent directory is a Python package. With the repository's default pytest import mode, pytest tests/gen_sim fails during collection with an import-file-mismatch error. Please rename this file (for example, test_scene_engine_cli_config.py) or make the test directories packages/use importlib mode.
| from pathlib import Path | ||
| from typing import Any, Iterable | ||
|
|
||
| import gradio as gr |
There was a problem hiding this comment.
Gradio is now a direct runtime dependency of this feature, but neither the core dependency list nor the gensim extra declares it. A clean pip install .[gensim] therefore cannot reliably launch this UI. Please add a supported Gradio version to an appropriate optional extra and document that installation path.
| /gym_project/ | ||
| .debug_engine/ | ||
|
|
||
| # Local Gradio UI dependencies, generated Articraft records, and bytecode |
3 participants
Description
This PR introduces an image-to-scene generation workflow for EmbodiChain, together with a Gradio-based workspace for running and previewing generative-simulation workflows.
Key changes:
Fixes # (issue)
Type of change
Checklist
black .command to format the code base.